Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid NPE in simplify #15310

Merged
merged 1 commit into from
May 30, 2022
Merged

Avoid NPE in simplify #15310

merged 1 commit into from
May 30, 2022

Conversation

odersky
Copy link
Contributor

@odersky odersky commented May 29, 2022

Fixes #15272

There's no crash anymore, but #15272 still does not compile.

Fixes scala#15272

There's no crash anymore, but scala#15272 still does not compile.
@odersky
Copy link
Contributor Author

odersky commented May 29, 2022

It's a bit troubling that this NPE occurred even with explicit null checking. I believe the root of the matter is that entries in OrderingConstraint is an Array[Type], but the array is only partially filled. The uninitialized elements are null. Is there a good way to check for this hole?

@noti0na1
Copy link
Member

noti0na1 commented May 29, 2022

It seems the correct type for ArrayValuedMap[T] is SimpleIdentityMap[TypeLambda, Array[T | Null]].

I think the correct to create Array is:

  • only create nullable array (Array[T | Null]) if there is no initial value
  • generate non-nullable array (Array[T]) if the initial values or default value generator (Int => T) is provided
  • a filter function which remove all null elements: def removeNullElem[T](xs: Array[T | Null]): Array[T]
  • a transform function from Array[T | Null] to Array[T] by checking every element not equal to null
  • and a unchecked version of transform function using .asInstanceOf[Array[T]] directly

But it's hard to do these without changing the standard library

@olhotak
Copy link
Contributor

olhotak commented May 30, 2022

The solution we had in mind way back when @abeln was working on explicit nulls was the following. I was sure we wrote it down somewhere but now I cannot find it:

  1. Use Array[T|Null] for arrays that can contain some uninitialized elements, and Array[T] (where T non-nullable) only in cases where we know that all elements of the array are initialized (with non-null values).
  2. Add an Array.ofNulls[T](n: Int): Array[T|Null] method that constructs an array of null values of a given size.
  3. Array.apply and Array.fill can still return non-nullable arrays because all the elements are provided at creation time, so don't have to be left null.
  4. Deprecate new Array(n: Int) in favour of point 2. above.

In this case, line 274 of OrderingConstraint.scala does:

    val entries1 = new Array[Type](nparams * 2)

This uses the to-be-deprecated point 4.

We can't do the deprecation without changing the standard library, but perhaps we could already implement 2. in Dotty itself, and modify existing code to avoid the to-be-deprecated point 4., to see how the resulting code looks. Specifically in this case, entries would have type Array[Type|Null] rather than just Array[Type].

@olhotak
Copy link
Contributor

olhotak commented May 30, 2022

Looking at this again, it's not clear to me whether the entries array is intended to contain null elements or whether the fact that it contains a null element is itself a bug in the logic.

If the former, its type should be Array[Type|Null]. If the latter, its type Array[Type] is correct, but then we need to determine how that null got in there.

@odersky
Copy link
Contributor Author

odersky commented May 30, 2022

The entries array can contain nulls in the second half where TypeVars are stored (or not).

@olhotak
Copy link
Contributor

olhotak commented May 30, 2022

OK. In that case, if we wanted to be safe here, we could make an opaque type for the ParamBounds arrays that returns non-nulls from the first half and nullable typeVars from the second half. But I looked into doing that and I don't think it's worth the complexity: in particular, it would mess up the ConstraintLens code that expects an array.

So I think we should just merge this as is.

@odersky odersky merged commit 9220b7e into scala:main May 30, 2022
@odersky odersky deleted the fix-15272 branch May 30, 2022 14:47
@griggt griggt added the backport:done This PR was successfully backported. label May 31, 2022
@Kordyjan Kordyjan added this to the 3.2.0 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:done This PR was successfully backported.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Matchtype recursion crashes
5 participants